Skip to content

Throw exception that was created but not thrown #6604

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Throw exception that was created but not thrown #6604

wants to merge 1 commit into from

Conversation

ThomasVitale
Copy link
Contributor

@ThomasVitale ThomasVitale commented Mar 10, 2019

Fixes #5462

In the DigestAuthenticationFilter, a BadCredentialsException was created when the nonce signature is invalid, but never thrown.

This case is already covered by a test (testNonceWithIncorrectSignatureForNumericFieldReturnsForbidden in the DigestAuthenticationFilterTests class), but it was green since an exception was thrown anyway at a later point of the filter, resulting in a 401 status (line 217).

)

@jzheaux
Copy link
Contributor

jzheaux commented Mar 21, 2019

@ThomasVitale good catch! I see your comment about an existing test.

Is there a test that you could add which would correctly fail before your change, but would pass afterward? It'd be nice to add that as part of this PR.

@ThomasVitale
Copy link
Contributor Author

ThomasVitale commented Mar 24, 2019

I thought about that, but I couldn't find a reasonable way to do it. The filter implementation can throw a BadCredentialsException for different reasons, but from the outside we are not aware of that since the filter catches the exception internally and returns a 401 response. And that is what is tested in DigestAuthenticationFilterTests. Do you have any suggestion? I would really much like to improve this PR with a better test.

@jzheaux
Copy link
Contributor

jzheaux commented Mar 25, 2019

Yeah, this is a bit of a tricky one. And digest auth is pretty complex on its own.

Thinking out loud here a bit, what we'd need is for this check:

String expectedNonceSignature = 
		DigestAuthUtils.md5Hex(this.nonceExpiryTime + ":" + entryPointKey);

if (!expectedNonceSignature.equals(nonceTokens[1])) {

to fail, but not any other check.

So, if the test's nonce were generated with something other than the configured entryPointKey, then I think that check would fail while the others would pass. Your test, for example, could call generateNonce, but not use KEY:

@Test
public void test() throws Exception {
	String badNonce = generateNonce(60, "badkey");
	String responseDigest = DigestAuthUtils.generateDigest(false, USERNAME, REALM,
			PASSWORD, "GET", REQUEST_URI, QOP, badNonce, NC, CNONCE);

	request.addHeader("Authorization", createAuthorizationHeader(USERNAME, REALM,
			badNonce, REQUEST_URI, responseDigest, QOP, NC, CNONCE));

	// ...
}

@jzheaux jzheaux self-assigned this Apr 14, 2019
@jzheaux jzheaux added type: enhancement A general enhancement in: web An issue in web modules (web, webmvc) labels Apr 14, 2019
@jzheaux jzheaux added this to the 5.2.0.M2 milestone Apr 14, 2019
@jzheaux
Copy link
Contributor

jzheaux commented Apr 14, 2019

Thanks again, @ThomasVitale, for the contribution! This is now merged into master via d88c2c1. I also added a unit test via 20a7bc4.

@jzheaux jzheaux closed this Apr 14, 2019
@ThomasVitale ThomasVitale deleted the gh-5462 branch June 10, 2019 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Created exception is not throwing
2 participants